Skip to content

Update local model downloading new lines #1163

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jun 25, 2020
Merged

Update local model downloading new lines #1163

merged 3 commits into from
Jun 25, 2020

Conversation

@deliahu
Copy link
Member

@deliahu deliahu commented Jun 25, 2020

@RobertLucian Does this change look ok to you? Do you know if we'd misformat something else by making this change?

Before:

$ cx deploy 

￮ downloading model s3://cortex-examples/tensorflow/image-classifier/inception/1569014553 . ✓

￮ downloading model s3://cortex-examples/tensorflow/iris-classifier/nn/1569001258 . ✓

￮ downloading model s3://cortex-examples/tensorflow/resnet50/1 . ✓

creating multi-model-classifier

cortex get                          (show api statuses)
cortex get multi-model-classifier   (show api info)
cortex logs multi-model-classifier  (stream api logs)

After:

$ cx deploy 

￮ downloading model s3://cortex-examples/tensorflow/image-classifier/inception/1569014553 . ✓
￮ downloading model s3://cortex-examples/tensorflow/iris-classifier/nn/1569001258 . ✓
￮ downloading model s3://cortex-examples/tensorflow/resnet50/1 . ✓

creating multi-model-classifier

cortex get                          (show api statuses)
cortex get multi-model-classifier   (show api info)
cortex logs multi-model-classifier  (stream api logs)

@deliahu deliahu requested a review from RobertLucian June 25, 2020 18:31
RobertLucian
RobertLucian approved these changes Jun 25, 2020
View reviewed changes
Copy link
Member

@RobertLucian RobertLucian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I like this more. This won't affect it negatively in any way.

@RobertLucian
Copy link
Member

RobertLucian commented Jun 25, 2020

If CacheModels function is ever to be called with no models, then the newline would mess up the output a bit. It's just better to check if any models have been printed to stdout and then print the newline as well.

Currently, CacheModels is only called when there are models to be cached, but still, it's better to be safe than sorry.

@deliahu deliahu merged commit 0748099 into master Jun 25, 2020
@deliahu deliahu deleted the local-model-newline branch June 25, 2020 20:12
deliahu added a commit that referenced this pull request Jun 28, 2020
@deliahu
Update local model downloading new lines (#1163)
6eb9f55 
(cherry picked from commit 0748099)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RobertLucian RobertLucian RobertLucian approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@deliahu @RobertLucian